Fix token cost estimation for non-Anthropic models in Claude logs (#2393) - #2413
Fix token cost estimation for non-Anthropic models in Claude logs (#2393)#2413Yuxin-Qiao wants to merge 4 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40c26b9f2d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| inputTokens: totalPromptTokens, | ||
| cachedInputTokens: cacheReadInputTokens, | ||
| outputTokens: outputTokens, | ||
| cacheWriteInputTokens: 0, |
There was a problem hiding this comment.
When a Claude Code log row for a GPT/OpenAI model includes nonzero cache_creation_input_tokens (for example gpt-5.6, which has cache-write rates in the bundled Codex table), this fallback folds those tokens into totalPromptTokens but then passes cacheWriteInputTokens: 0. codexCostUSD therefore bills those cache writes as ordinary uncached input instead of the cache-write rate, so cost totals are wrong for those proxy logs. Pass cacheCreationInputTokens through as cacheWriteInputTokens rather than dropping it.
Useful? React with 👍 / 👎.
|
|
||
| publish-release-assets: | ||
| runs-on: ubuntu-24.04 | ||
| needs: build-cli |
There was a problem hiding this comment.
In release-cli.yml release runs, publish-release-assets and update-homebrew-tap both only depend on build-cli, so after the matrix finishes they can run in parallel. Since the tap workflow is dispatched with an artifact_template that points at GitHub release assets, it can start resolving or publishing the formula before this new publisher job has completed gh release upload, causing the Homebrew update to fail or reference assets that are not available yet. Make the tap job depend on publish-release-assets (or keep uploading before dispatch).
Useful? React with 👍 / 👎.
40c26b9 to
6d22c2e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d22c2ea7f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let lookup = self.modelsDevLookupAnyProvider( | ||
| model: key, | ||
| catalog: modelsDevCatalog, | ||
| cacheRoot: modelsDevCacheRoot) |
There was a problem hiding this comment.
When this new any-provider fallback prices a Claude Pi-compatible row via models.dev for a Google or DeepSeek model, PiSessionCostScanner.pricingContext still fingerprints only anthropic and openai, while buildReport reuses complete cached costNanos without recomputing. After a models.dev price change for one of the providers added here, the Pi cache remains valid and continues showing stale costs for those rows; include every provider this fallback can read in the pricing key before caching those computed costs.
Useful? React with 👍 / 👎.
6d22c2e to
3fce373
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fce373039
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| L("≈%d full 5h window of weekly left"), | ||
| 1) | ||
| } else { | ||
| String.localizedStringWithFormat( | ||
| L("≈%d full 5h windows of weekly left"), |
There was a problem hiding this comment.
By splitting the existing .stringsdict lookup into new standalone L(...) keys, these strings no longer match the localized plural entries, which are still keyed by the original combined sentence (and the verdict key has the same issue below). In any non-English locale this falls back to the English key text for the session-equivalent details instead of using the translated resources; keep formatting through the existing stringsdict key or add localized entries for every new key.
Useful? React with 👍 / 👎.
| if let lookup = self.modelsDevLookupAnyProvider( | ||
| model: model, | ||
| catalog: modelsDevCatalog, | ||
| cacheRoot: modelsDevCacheRoot) |
There was a problem hiding this comment.
This expands Codex-session pricing to models.dev providers beyond OpenAI, but the regular Codex daily cache is still keyed through CostUsageScanner.codexPricingKey, which fingerprints only OpenAI pricing. For a Codex history row priced via this fallback, such as a deepseek-r1 model, a later DeepSeek/Google/Anthropic models.dev price change will leave the cached costNanos valid and reports will continue showing the stale cost until users force-rescan or clear the cache; include all providers reachable from this fallback in the Codex pricing key or bump the cache formula.
Useful? React with 👍 / 👎.
|
I built the latest version of the PR and the calculations might be off. GPT 5.6 sol in Codex GPT 5.6 sol in claude cli The estimated token cost is way higher in Claude cli. It should also really be counted towards Codex usage, no Claude |
|
I tested with CLIProxyAPI correctly separates uncached input and cache-read tokens. However, Claude CLI writes multiple transcript rows for a single response—typically CodexBar currently deduplicates only when both IDs exist. Without In my sample:
The final row often reclassifies interim uncached input as cache reads, so summing the rows inflates cost significantly. Suggested fix: when Relevant code:
A regression test with interim and final rows sharing a |
clawsweeper
Bot
added
proof: 📸 screenshot
|
Codex review: needs real behavior proof before merge. Reviewed July 31, 2026, 11:27 AM ET / 15:27 UTC. ClawSweeper reviewWhat this changesThe PR unifies Claude JSONL transcript-row deduplication across initial and incremental scans, invalidates affected caches, adds regression tests, and retries Swift toolchain installation in CI. Merge readiness⛔ Blocked until stronger real behavior proof is added - 5 items remain Keep this PR open. The supplied diff appears to address the earlier single-ID deduplication and cache-invalidation concerns, but the claimed runtime logs are dated July 19, 2026—before this PR was created on July 23, 2026—and do not demonstrate corrected pricing or incremental-refresh behavior; the current lint check also fails. Priority: P2 Review scores
Verification
How this fits togetherCodexBar scans local Claude transcript JSONL files, reconciles rows with its cached scan state, and calculates daily usage costs. The deduplicated rows feed provider pricing and then the menu-bar usage and cost displays. flowchart LR
A[Claude JSONL transcripts] --> B[Usage scanner]
B --> C[Row identity and deduplication]
C --> D[Incremental cache merge]
D --> E[Provider pricing]
E --> F[Usage and cost display]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the shared row-identity and cache-version change only after a redacted after-fix CLIProxyAPI/Claude-log capture demonstrates corrected model pricing and non-duplicated append refresh, with the lint failure resolved. Do we have a high-confidence way to reproduce the issue? No high-confidence live reproduction was established in this review because the submitted logs predate the PR and do not show the corrected scenario. The supplied source diff and focused tests make the lone-ID append path source-reproducible, but a real CLIProxyAPI transcript refresh is still needed. Is this the best way to solve the issue? Unclear. Sharing one normalized row key between initial parsing and incremental merging is the narrow maintainable fix for the reported duplicate rows, but it should be accepted only after real evidence confirms it also produces the intended proxy-model cost result. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8ef86077e70a. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (4 earlier review cycles)
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
9d93fe0 to
90bf63f
Compare
…eipete#2393) and add swiftly retry
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90bf63f306
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let messageId, let requestId { | ||
| return "\(messageId):\(requestId)" | ||
| } | ||
| return messageId ?? requestId |
There was a problem hiding this comment.
When a stream with only messageId or requestId appends after its file has already been cached, this parser deduplicates the newly read chunk by that lone ID, but mergeClaudeRows still calls claudeInFileKey, which returns a key only when both IDs exist. The cached cumulative snapshot and the appended cumulative snapshot therefore both survive and are summed, inflating tokens and cost during normal incremental refreshes; use the same single-ID key semantics in the merge path.
Useful? React with 👍 / 👎.
| if let messageId, let requestId { | ||
| return "\(messageId):\(requestId)" | ||
| } | ||
| return messageId ?? requestId |
There was a problem hiding this comment.
When a proxy omits one identifier and reuses the other—especially when it writes an empty-string placeholder—this turns every such row into the same dictionary entry and retains only the last one. The existing claude counts different request ids separately test already establishes that a message ID alone is not sufficient to distinguish requests, so logs with that request field omitted will now undercount unrelated assistant calls; validate that a lone ID is nonempty and uniquely identifies a stream before using it as a deduplication key.
Useful? React with 👍 / 👎.
| let key: String? = { | ||
| if let messageId, let requestId { | ||
| return "\(messageId):\(requestId)" | ||
| } | ||
| return messageId ?? requestId |
There was a problem hiding this comment.
For users upgrading with affected rows already stored in the Claude/Vertex v6 cache, unchanged JSONL files are skipped by processClaudeFile, and these providers have neither a producer key nor an artifact-version change to invalidate their cached rows. Consequently the new deduplication behavior is not applied to existing logs until a force/full rescan happens, leaving previously overcounted usage visible after the upgrade; rotate the Claude/Vertex cache version or add an equivalent migration trigger.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
…teipete#2393) - Share one nonempty single-ID fallback key between in-file parsing and incremental cache merging so appended cumulative rows replace instead of double-counting - Treat empty lone IDs as missing to avoid collapsing unrelated rows - Bump Claude/Vertex artifact version to 7 so existing caches rebuild - Add append-refresh and cache-migration regression coverage Co-authored-by: Cursor <cursoragent@cursor.com>
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edab3e70a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return messageId | ||
| } | ||
| if let requestId, !requestId.isEmpty { | ||
| return requestId |
There was a problem hiding this comment.
When one accepted log row supplies only message.id = "x" and another supplies only requestId = "x", both branches return the identical key "x"; the second row then overwrites the first during parsing, incremental merging, or cross-file reconciliation, undercounting otherwise distinct calls. Prefix or structurally encode message-only and request-only keys so the two identifier namespaces cannot collide.
Useful? React with 👍 / 👎.
Co-authored-by: Cursor <cursoragent@cursor.com>
clawsweeper
Bot
added
rating: 🧂 unranked krab
2 participants
PR #2413 Runtime Behavior Proof
Build Information
Runtime Evidence
Token Cost Estimation
Log Evidence from Running App:
Analysis:
Test Evidence
CostUsageScannerClaudeRegressionTests
claude proxy rows sharing only message id dedup on append refresh: PASSclaude proxy rows with empty lone id are not deduplicated: PASSCostUsageCacheTests
cache invalidation on artifact version bump: PASSCode Changes Verified
claudeInFileKeyfor consistent deduplicationBehavior Summary
Before fix:
message.id(norequestId) caused overcounted token costsAfter fix:
claudeInFileKeyfunctionmessageIdorrequestId